Stop reporting expected Admin API failures from store execute as CLI bugs#7406
Open
Stop reporting expected Admin API failures from store execute as CLI bugs#7406
Conversation
Contributor
Author
This stack of pull requests is managed by Graphite. Learn more about stacking. |
dmerand
force-pushed
the
dlm-store-execute-error-classify
branch
10 times, most recently
from
ddfc7b3 to
02e2b80
Compare
Contributor
There was a problem hiding this comment.
Pull request overview
This PR reduces error-analytics noise for shopify store execute by avoiding cli-kit's fetchApiVersions “unknown error” wrapping and instead performing Admin API version discovery via the same graphqlRequest primitive used for the execute-phase request, enabling consistent error-shape classification.
Changes:
- Added a store-execute–local
fetchPublicApiVersions(Admin GraphQL) and centralized error classification for 402 (store unavailable) and aborted fetches. - Updated context creation to use the new transport function for version resolution (keeping orchestration in
admin-context.ts). - Expanded/added unit tests to cover both transport functions and the relevant classification branches.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| packages/store/src/cli/services/store/execute/admin-transport.ts | Adds version-discovery GraphQL request + shared error classifier to map 402/abort shapes to user-facing AbortErrors and preserve stored-auth reauth behavior. |
| packages/store/src/cli/services/store/execute/admin-transport.test.ts | Adds comprehensive transport-level test coverage for success paths, auth invalidation, 402 ordering, and aborted-fetch classification. |
| packages/store/src/cli/services/store/execute/admin-context.ts | Switches version resolution from cli-kit fetchApiVersions to the new transport-owned fetchPublicApiVersions. |
| packages/store/src/cli/services/store/execute/admin-context.test.ts | Updates tests to mock fetchPublicApiVersions and focus coverage on orchestration behavior. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…bugs Two error shapes from the Admin API were being misclassified as `BugError` and reported to Observe (issue shop/issues#32996): - HTTP 402 "Unavailable Shop" \u2014 the shop is frozen / on hold; this is a store-state issue, not a CLI bug. - "The user aborted a request." \u2014 surfaces from node-fetch when an AbortController signal fires (CLI-side fetch timeout or user cancel). Both were getting wrapped by `fetchApiVersions`'s "unknown error" catch in cli-kit, which destroys the original error type. To classify them locally without reaching past the public API, `store execute` now drives its API-version lookup directly via `graphqlRequest` against an inline `publicApiVersions` query \u2014 the same primitive `admin-transport.ts` uses for the user's query. Both phases now see raw `ClientError` / `Error` shapes and share a single classifier in `admin-errors.ts`. Other commands that hit the Admin API on startup are unaffected; cli-kit is untouched. - `packages/store/.../execute/admin-errors.ts` (new): structural `ClientError` shape check, abort detection, and `classifyAdminApiError` that maps known non-bug shapes to user-facing `AbortError`s. - `packages/store/.../execute/admin-context.ts`: drives the version lookup via `graphqlRequest` directly. 401/404 still trigger the re-auth path (now via structural status check rather than parsing message strings); 402 and aborts go through the shared classifier. - `packages/store/.../execute/admin-transport.ts`: same shared classifier covers aborts on the execute-phase request. New `admin-errors.test.ts` covers the classifier; existing context and transport tests updated to mock `graphqlRequest` directly.
dmerand
force-pushed
the
dlm-store-execute-error-classify
branch
from
02e2b80 to
4b99cef
Compare
isaacroldan
reviewed
Apr 28, 2026
| // node-fetch surfaces) so a fetch that bubbles past cli-kit's retry layer is still | ||
| // classified correctly. Exported so tests reference the same source of truth as | ||
| // production. | ||
| export const ABORTED_FETCH_MESSAGE_FRAGMENTS = ['the user aborted a request', 'the operation was aborted'] as const |
Contributor
There was a problem hiding this comment.
We already have errorMessageImpliesEnvironmentIssue in error.ts in cli-kit, maybe you want to reuse that?
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
WHY are these changes introduced?
Fixes shop/issues#32996.
shopify store executeis creating Observe error-analytics issues (403 events / 113 affected users in the last 7 days) for two error shapes that are not CLI bugs:GraphQL Error (Code: 402): "Unavailable Shop"— the shop is frozen / on hold; this is a store-state issue.The user aborted a request.— surfaces when anAbortControllersignal fires (CLI-side fetch timeout or user cancel).Both were ending up wrapped by
fetchApiVersions's "unknown error" branch in cli-kit, which throwsBugError("Unknown error connecting…")and destroys the original error's type. The version lookup runs on everystore executestartup, so both shapes show up reliably as bugs in error analytics.WHAT is this pull request doing?
The fix is local to
store execute— that's where the Observe noise was reported, and the recovery copy is store-flavored. Other Admin API callers (app, theme, hydrogen) keep their existing behavior. cli-kit is untouched.store executenow drives its API-version lookup directly viagraphqlRequestagainst an inlinepublicApiVersionsquery — the same primitive the execute-phase request already uses. With both phases on the same primitive, they see the same rawClientError/Errorshapes, so a single shared classifier (file-private toadmin-transport.ts) covers both.store/.../execute/admin-context.tsstore/.../execute/admin-transport.tsfetchPublicApiVersions+runAdminStoreGraphQLOperation). 402 →AbortError("The store … is currently unavailable."); user-aborted fetches →AbortError("Request to … was aborted before it completed."); 401/404 still trigger the existing stored-auth re-auth path via structural status check.store/.../execute/admin-context.test.tsfetchPublicApiVersions; covers orchestration only.store/.../execute/admin-transport.test.tsABORTED_FETCH_MESSAGE_FRAGMENTSso adding a new abort shape auto-extends coverage). Includes a 402-with-errorscase that locks down the branch ordering.Behavior preserved from before this PR: when the Admin API returns a GraphQL
errorspayload,runAdminStoreGraphQLOperationthrowsAbortError("GraphQL operation failed.")— process exits non-zero. This is unchanged. The closest sibling,packages/app/.../execute-operation.ts, instead renders GraphQL errors viarenderErrorand returns void (zero exit). Aligning the two is out of scope for this PR; we keepstore execute’s existing exit-code semantics.How to test your changes?
Unit tests:
The
admin-transport.test.tssuite covers every error shape from the bug report: 402 in the versions phase, 402 witherrors: [...]in the execute phase, each abort-message fragment, abort-by-name, and 401 in both phases.Manual smoke (against any working store, to confirm no regression):
To exercise the new 402 branch you need a frozen / unavailable store; if you have one, the CLI should surface a clean
AbortError("The store … is currently unavailable.")with recovery guidance instead of the previousBugError("Unknown error connecting…").Post-release steps
None.
Checklist
clislice forstore execute; visible within a few days post-release)BugErrorto anAbortErrorwith cleaner copy). Existing@shopify/storeconsumers see the same exit code on the same input; they were already trapping or reporting these errors. The user-facing improvement is the reduced Observe noise / cleaner CLI message, not a published API change.